fix build and run on OpenBSD #6050
Conversation
…ordingly to avoid read overflow. To revisit after further investigation.
|
Please run |
|
I fixed the offending line by hand, will try run-clang-format.sh. Does this also apply to Starship, libultraship ... ? The clang version I have 19.1.7, after runing run-clang-format.sh bring in more changes : |
|
We use clang 14.0.6 specifically, that's why it made more changes than it fixed. It does apply to 2ship and LUS as well, not sure about Starship or Spaghetti Kart. |
soh/src/code/audio_synthesis.c
Outdated
| aligned = ALIGN16((nFramesToDecode * frameSize) + 16); | ||
| addr = DMEM_COMPRESSED_ADPCM_DATA - aligned; | ||
|
|
||
| #if __SANITIZE_ADDRESS__ || defined(__OpenBSD__) |
There was a problem hiding this comment.
when this is unconditional the audio issue still only happens on OpenBSD I assume
wondering why this can't either be unconditional, or if there's some alignment handling aLoadBuffer should be using instead
which is to say I don't really understand what's happening here, & would appreciate explanation
There was a problem hiding this comment.
The SANITIZE_ADDRESS came from Starship.
Check HarbourMasters/Starship@64442db
I'm mainly debugging there (in Starship) atm. But the following principle still stands and both are very similar.
The CODEC_ADPCM uses 9 bytes to decode into 16 samples. The first byte is important as it provides a hint on how to decode the following 8 bytes into 16 samples. Which means we can't decode half of a frame easily.
The function AudioSynth_ProcessNote calculates a number of samples to process but this isn't aligned to frames, thus the need for some math around (which I don't fully understand yet). In some cases this goes wrong and it tries to decode bytes outside the audioFontSample. OpenBSD is more strict so the software will crash early. Note it doesn't crash on all audioFontSample and I guess it's because of how the memory is allocated vs the size of the book which means there is room after the end of buffer and before the end of memory allocated. But still it's an out of bound read overflow.
My hypothesis is you will start noticing choppy audio on Linux if you enable the this check unconditionally.
I do not have Linux at hand so I can't compare easily. Maybe you could add a printf to check if Linux actually triggers the check ? Otherwise, it would mean something else deviated on OpenBSD and led to this crash.
There was a problem hiding this comment.
For information.
2ship have a similar issue, see comments in https://github.com/HarbourMasters/2ship2harkinian/blob/6dc782221eb46432f35bfb4cf3f64d70c0b8b640/mm/src/audio/lib/synthesis.c#L1212
SpaghettiKart have an unconditional bound check in https://github.com/HarbourMasters/SpaghettiKart/blob/186ea294aedd05efc9ab799507dd96040a05741c/src/audio/synthesis.c#L488
There was a problem hiding this comment.
does #6089 help? it has some fixes for audio samples
There was a problem hiding this comment.
Didn't try the diff yet, it will fix the crash for sure with the extra padding.
From what I remember, it's a matter of a frame so at most 16 samples which are 9 bytes encoded.
Also, it memset to 0 so I'm curious which sound it will produce.
I doubt it will be exactly like intended but maybe ... will test for sure.
Also, this is the place to pre-decode samples in memory, maybe that would be easier (I guess).
Thanks for the hint.
There was a problem hiding this comment.
I revert SANITIZE_ADDRESS stuff.
Recently I realised the aLoadBufferImpl use ROUND_DOWN_16.
Line 103 in a92f07e
All games should at least align to 16 when allocating ADPCM bytecode.
I have no idea why there is that constraint on alignment.
From my ears it looks better now (less choppy) but still not correct (noisy, feels like choppy).
There is also skipInitialSamples which may push the buffer too far. I have no idea why it exists at start.
The extra 32 padding in AudioSampleFactory.cpp may not be needed.
Shipwright/soh/src/code/audio_synthesis.c
Line 883 in a92f07e
From my ears it doesn't seem better with the bellow change.
- frameIndex = (synthState->samplePosInt + skipInitialSamples - nFirstFrameSamplesToIgnore) / 16;
+ frameIndex = synthState->samplePosInt / 16;
I think I need to record and compare on another OS.
I mean I need to narrow why the sound isn't as good as it should be (compared to emulation).
There was a problem hiding this comment.
Yep, these basic fixes are good, the audio stuff is better in its own PR when there's a better handle as to what's going on
…gned accordingly to avoid read overflow. To revisit after further investigation." Fix with PR HarbourMasters#6089 This reverts commit 0e6003e.
3 participants
Just like Apple, OpenBSD doesn't provide malloc.h header and already defines __assert() from the base system.
Hopefully the offending __assert() which doesn't have the same arguments isn't used, otherwise it would error out.
The cpp compiler on OpenBSD is more strict and raise an error on invalid operands to binary expression.
('uintptr_t' (aka 'unsigned long') and 'std::nullptr_t')
48 | if (desiredTarget != NULL)
| ~~~~~~~~~~~~~ ^ ~~~~
Finally, Shipwright has the same memory/alignment issue seen in Starship (which already have a workaround guarded by SANITIZE_ADDRESS). See HarbourMasters/Starship#238.
Build Artifacts